Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLD: pin pyqt to 5.x #999

Merged
merged 3 commits into from
May 23, 2023
Merged

BLD: pin pyqt to 5.x #999

merged 3 commits into from
May 23, 2023

Conversation

klauer
Copy link
Collaborator

@klauer klauer commented May 18, 2023

Remove the <5.15 pin and just ensure we're on PyQt 5.x.

This is just a test and may fail miserably. Let's see what happens on CI.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2023

Codecov Report

Merging #999 (13e5c1e) into master (504e084) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   59.21%   59.20%   -0.01%     
==========================================
  Files         105      105              
  Lines       14054    14054              
==========================================
- Hits         8322     8321       -1     
- Misses       5732     5733       +1     

see 1 file with indirect coverage changes

@klauer klauer marked this pull request as ready for review May 22, 2023 18:07
@klauer
Copy link
Collaborator Author

klauer commented May 22, 2023

So it pretty much works, according to CI testing. A quick double-check indicates that 5.15 is getting installed there:

pyqt                      5.15.7           py38ha0d8c90_3    conda-forge

On the PCDS side, we have a desire to use 5.15 for our general runtime/data acquisition environment. As we all know at this point, if you want Qt Designer to work with PyDM plugins, you have to pin to another specific version due to conda-forge's removal of a necessary shared library in recent releases. For that, we intend to keep a designer-only environment available that allows screen development to continue.

So, if PyDM's <5.15 pinning is relaxed to =5, we'll be free to adjust our pyqt pinning at the environment level.

I'll mark this as ready for review to get input. Would this PR or the above environment creation methodology be problematic for the accelerator (or other PyDM users)?

@ZLLentz
Copy link
Member

ZLLentz commented May 22, 2023

My thoughts are:

  • Is there any reason to keep the pin in order to make sure people get the pyqt builds with proper designer support?
  • After removing this pin, do we need to test against multiple versions of pyqt to make sure we don't stumble into regressions for people who are running the older versions with proper designer support?

Otherwise, I agree with the change. Extraneous upper pins cause more problems than they solve.

@jbellister-slac
Copy link
Collaborator

jbellister-slac commented May 22, 2023

Would this PR or the above environment creation methodology be problematic for the accelerator (or other PyDM users)?

Is there any reason to keep the pin in order to make sure people get the pyqt builds with proper designer support?

There would be no issue on the accelerator side, we can pin at the environment level as well. And the workaround of just copying over the libpyqt5.so file as mentioned near the end of #866 still works for now at least, though not exactly robust.

Maybe a note added to our installation documentation with the command for creating a designer compatible environment would be helpful, especially for other pydm users as well. (I could add as a followup if you'd like)

After removing this pin, do we need to test against multiple versions of pyqt to make sure we don't stumble into regressions for people who are running the older versions with proper designer support?

This sounds like a good idea. I want to take a look at our pipelines already (maybe switching to mamba at least to fix the long build issues, maybe move to github actions entirely). Adding old and new pyqt could be done there as well.

Overall though I agree with the change as well, gives users the choice whether to pin or not as needed.

Copy link
Collaborator

@jbellister-slac jbellister-slac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the suggestions block this being done though, so I am good with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants